core: Ensure hasComputedSubKeys iterates over Sets and Lists properly #9171

vancluever · 2016-10-02T18:15:18Z

WIP - Adding tests to this :)

This fixes some edge-ish cases where a set in a config has a set or list
in it that contains computed values, but non-set or list values in the
parent do not.

This can cause "diffs didn't match during apply" errors in a scenario
such as when a set's hash is calculated off of child items (including
any sub-lists or sets, as it should be), and the hash changes between
the plan and apply diffs due to the computed values present in the
sub-list or set items. These will be marked as computed, but due to the
fact that the function was not iterating over the list or set items
properly (ie: not adding the item number to the address, so
set.0.set.foo was being yielded instead of set.0.set.0.foo), these
computed values were not being properly propagated to the parent set to
be marked as computed.

Fixes #6527.
Fixes #8271.

This possibly fixes other non-CloudFront related issues too.

vancluever · 2016-10-02T20:58:46Z

About to remove the WIP, however going to throw a test that does not see to be working in the gist below:

https://gist.github.com/vancluever/b0c52e5da0193d6a24b0a326d7117c6c

The test fails with:

--- FAIL: TestSchemaMap_Diff (0.00s)
    schema_test.go:2658: #"Complex structure with list of computed string should mark root set as computed":

        expected:
        *terraform.InstanceDiff{mu:sync.Mutex{state:0, sema:0x0}, Attributes:map[string]*terraform.ResourceAttrDiff{"outer.~1.inner.#":*terraform.ResourceAttrDiff{Old:"0", New:"1", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "outer.~1.inner.0":*terraform.ResourceAttrDiff{Old:"", New:"${var.bar}", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "outer.#":*terraform.ResourceAttrDiff{Old:"0", New:"1", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "outer.~1.outer_str":*terraform.ResourceAttrDiff{Old:"", New:"foo", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}}, Destroy:false, DestroyTainted:false}

        got:
        *terraform.InstanceDiff{mu:sync.Mutex{state:0, sema:0x0}, Attributes:map[string]*terraform.ResourceAttrDiff{"outer.#":*terraform.ResourceAttrDiff{Old:"0", New:"1", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "outer.~1.outer_str":*terraform.ResourceAttrDiff{Old:"", New:"foo", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "outer.~1.inner.#":*terraform.ResourceAttrDiff{Old:"0", New:"", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}}, Destroy:false, DestroyTainted:false}

@radeksimko maybe related to #7485 and #7393?

This fixes some edge-ish cases where a set in a config has a set or list in it that contains computed values, but non-set or list values in the parent do not. This can cause "diffs didn't match during apply" errors in a scenario such as when a set's hash is calculated off of child items (including any sub-lists or sets, as it should be), and the hash changes between the plan and apply diffs due to the computed values present in the sub-list or set items. These will be marked as computed, but due to the fact that the function was not iterating over the list or set items properly (ie: not adding the item number to the address, so set.0.set.foo was being yielded instead of set.0.set.0.foo), these computed values were not being properly propagated to the parent set to be marked as computed. Fixes #6527. Fixes #8271. This possibly fixes other non-CloudFront related issues too.

This covers: * Complex sets with computed fields in a set * Complex lists with computed fields in a set Adding a test to test basic lists with computed fields seemed to fail, but possibly for an unrelated reason (the list returned as nil). The fix to this inparticular case may be out of the scope of this specific issue. Reference gist and details in #9171.

@mitchellh

Needed due to work done in 95d37ea, we may need to adjust hasComputedSubKeys to propagate NewComputed in the same way that we have added "~", however will wait for comment from @mitchellh.

vancluever · 2016-11-19T17:34:03Z

I've updated this PR with a a rebase and a slight update to the tests.

@mitchellh, I noticed that the necessary updates to the tests were probably needed due to work done in 95d37ea and related commits - is this PR still relevant in the face of that work?

Here's an example of a one of the failing tests post-commit:

    --- FAIL: TestSchemaMap_DiffSuppress/Complex_structure_with_complex_list_of_computed_string_should_mark_root_set_as_computed (0.00s)
        schema_test.go:3444: #"Complex structure with complex list of computed string should mark root set as computed":

            expected:
            (*terraform.InstanceDiff)(0xc4204bea40)({
             mu: (sync.Mutex) {
              state: (int32) 0,
              sema: (uint32) 0
             },
             Attributes: (map[string]*terraform.ResourceAttrDiff) (len=4) {
              (string) (len=7) "outer.#": (*terraform.ResourceAttrDiff)(0xc42033c780)({
               Old: (string) (len=1) "0",
               New: (string) (len=1) "1",
               NewComputed: (bool) false,
               NewRemoved: (bool) false,
               NewExtra: (interface {}) <nil>,
               RequiresNew: (bool) false,
               Sensitive: (bool) false,
               Type: (terraform.DiffAttrType) 0
              }),
              (string) (len=18) "outer.~1.outer_str": (*terraform.ResourceAttrDiff)(0xc42033c7c0)({
               Old: (string) "",
               New: (string) (len=3) "foo",
               NewComputed: (bool) false,
               NewRemoved: (bool) false,
               NewExtra: (interface {}) <nil>,
               RequiresNew: (bool) false,
               Sensitive: (bool) false,
               Type: (terraform.DiffAttrType) 0
              }),
              (string) (len=16) "outer.~1.inner.#": (*terraform.ResourceAttrDiff)(0xc42033c800)({
               Old: (string) (len=1) "0",
               New: (string) (len=1) "1",
               NewComputed: (bool) false,
               NewRemoved: (bool) false,
               NewExtra: (interface {}) <nil>,
               RequiresNew: (bool) false,
               Sensitive: (bool) false,
               Type: (terraform.DiffAttrType) 0
              }),
              (string) (len=26) "outer.~1.inner.0.inner_str": (*terraform.ResourceAttrDiff)(0xc42033c840)({
               Old: (string) "",
               New: (string) (len=10) "${var.bar}",
               NewComputed: (bool) false,
               NewRemoved: (bool) false,
               NewExtra: (interface {}) <nil>,
               RequiresNew: (bool) false,
               Sensitive: (bool) false,
               Type: (terraform.DiffAttrType) 0
              })
             },
             Destroy: (bool) false,
             DestroyTainted: (bool) false
            })


            got:
            (*terraform.InstanceDiff)(0xc420412840)({
             mu: (sync.Mutex) {
              state: (int32) 0,
              sema: (uint32) 0
             },
             Attributes: (map[string]*terraform.ResourceAttrDiff) (len=4) {
              (string) (len=7) "outer.#": (*terraform.ResourceAttrDiff)(0xc420457c80)({
               Old: (string) (len=1) "0",
               New: (string) (len=1) "1",
               NewComputed: (bool) false,
               NewRemoved: (bool) false,
               NewExtra: (interface {}) <nil>,
               RequiresNew: (bool) false,
               Sensitive: (bool) false,
               Type: (terraform.DiffAttrType) 0
              }),
              (string) (len=18) "outer.~1.outer_str": (*terraform.ResourceAttrDiff)(0xc420457dc0)({
               Old: (string) "",
               New: (string) (len=3) "foo",
               NewComputed: (bool) false,
               NewRemoved: (bool) false,
               NewExtra: (interface {}) <nil>,
               RequiresNew: (bool) false,
               Sensitive: (bool) false,
               Type: (terraform.DiffAttrType) 0
              }),
              (string) (len=16) "outer.~1.inner.#": (*terraform.ResourceAttrDiff)(0xc420457f80)({
               Old: (string) (len=1) "0",
               New: (string) (len=1) "1",
               NewComputed: (bool) false,
               NewRemoved: (bool) false,
               NewExtra: (interface {}) <nil>,
               RequiresNew: (bool) false,
               Sensitive: (bool) false,
               Type: (terraform.DiffAttrType) 0
              }),
              (string) (len=26) "outer.~1.inner.0.inner_str": (*terraform.ResourceAttrDiff)(0xc420426080)({
               Old: (string) "",
               New: (string) (len=10) "${var.bar}",
               NewComputed: (bool) true,
               NewRemoved: (bool) false,
               NewExtra: (interface {}) <nil>,
               RequiresNew: (bool) false,
               Sensitive: (bool) false,
               Type: (terraform.DiffAttrType) 0
              })
             },
             Destroy: (bool) false,
             DestroyTainted: (bool) false
            })

One thing that I'm noticing here too is that maybe where we are marking the parents with ~, we might need to mark NewComputed too - thoughts? For now I've added it to the inner parts to ensure the tests pass.

mitchellh · 2016-11-22T01:33:50Z

@vancluever Great job, great tests.

I added some tests to terraform/diff_test.go that do the "diff mismatch" test logic to verify that it views the two diffs as the same (and not a mismatch). And it seems to pass. I reverted your subkey logic since it seems the other work makes that pass (your tests unchanged).

I think as you said having NewComputed on the outer key MAY be more correct but at the moment this makes a fully working thing (according to the tests) so that may be an optimization we want to make in the future but for now might be okay.

Thanks for your work on this!

vancluever · 2016-11-22T04:52:35Z

@mitchellh fair enough and thanks for the merge and the feedback! Glad to help as always :)

This covers: * Complex sets with computed fields in a set * Complex lists with computed fields in a set Adding a test to test basic lists with computed fields seemed to fail, but possibly for an unrelated reason (the list returned as nil). The fix to this inparticular case may be out of the scope of this specific issue. Reference gist and details in hashicorp#9171.

ghost · 2020-04-13T02:06:51Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

vancluever changed the title ~~[WIP] core: Ensure hasComputedSubKeys iterates over Sets and Lists properly~~ core: Ensure hasComputedSubKeys iterates over Sets and Lists properly Oct 2, 2016

stack72 added core bug labels Oct 3, 2016

vancluever added 3 commits November 19, 2016 08:56

core: Backport NewComputed change to nested list/set tests

8d06d68

Needed due to work done in 95d37ea, we may need to adjust hasComputedSubKeys to propagate NewComputed in the same way that we have added "~", however will wait for comment from @mitchellh.

mitchellh merged commit 8d06d68 into hashicorp:master Nov 22, 2016

mitchellh added a commit that referenced this pull request Nov 22, 2016

terraform: add more Same test cases to cover #9171

71a1e21

gusmat pushed a commit to gusmat/terraform that referenced this pull request Dec 6, 2016

terraform: add more Same test cases to cover hashicorp#9171

604d1e5

fatmcgav pushed a commit to fatmcgav/terraform that referenced this pull request Feb 27, 2017

terraform: add more Same test cases to cover hashicorp#9171

8074057

vancluever deleted the fix-field-reader-setsubkeys-computed branch April 29, 2017 16:25

ghost locked and limited conversation to collaborators Apr 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Ensure hasComputedSubKeys iterates over Sets and Lists properly #9171

core: Ensure hasComputedSubKeys iterates over Sets and Lists properly #9171

vancluever commented Oct 2, 2016

vancluever commented Oct 2, 2016

vancluever commented Nov 19, 2016

mitchellh commented Nov 22, 2016

vancluever commented Nov 22, 2016

ghost commented Apr 13, 2020

core: Ensure hasComputedSubKeys iterates over Sets and Lists properly #9171

core: Ensure hasComputedSubKeys iterates over Sets and Lists properly #9171

Conversation

vancluever commented Oct 2, 2016

vancluever commented Oct 2, 2016

vancluever commented Nov 19, 2016

mitchellh commented Nov 22, 2016

vancluever commented Nov 22, 2016

ghost commented Apr 13, 2020